[libc++] Replace __compressed_pair with [[no_unique_address]]
#76756
Conversation
llvmbot
added
the
libc++
|
CC @cjdb |
philnik777
added
enhancement
|
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis significantly simplifies the code, improves compile times and improves the object layout of types using Patch is 138.56 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/76756.diff 21 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index adff13e714cb64..e6d167140c99a0 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -168,6 +168,8 @@
// pointer from 16 to 8. This changes the output of std::string::max_size,
// which makes it ABI breaking
# define _LIBCPP_ABI_STRING_8_BYTE_ALIGNMENT
+// Don't add padding from __compressed_pair
+# define _LIBCPP_ABI_NO_COMPRESSED_PAIR_PADDING
# elif _LIBCPP_ABI_VERSION == 1
# if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
// Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index 6505bb5871739d..e8c2b5040a7ddc 100644
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -138,45 +138,46 @@ class __default_alloc_func;
template <class _Fp, class _Ap, class _Rp, class... _ArgTypes>
class __alloc_func<_Fp, _Ap, _Rp(_ArgTypes...)> {
- __compressed_pair<_Fp, _Ap> __f_;
+ _LIBCPP_COMPRESSED_PAIR(_Fp, __func_, _Ap, __alloc_);
public:
typedef _LIBCPP_NODEBUG _Fp _Target;
typedef _LIBCPP_NODEBUG _Ap _Alloc;
- _LIBCPP_HIDE_FROM_ABI const _Target& __target() const { return __f_.first(); }
+ _LIBCPP_HIDE_FROM_ABI const _Target& __target() const { return __func_; }
// WIN32 APIs may define __allocator, so use __get_allocator instead.
- _LIBCPP_HIDE_FROM_ABI const _Alloc& __get_allocator() const { return __f_.second(); }
+ _LIBCPP_HIDE_FROM_ABI const _Alloc& __get_allocator() const { return __alloc_; }
- _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f)
- : __f_(piecewise_construct, std::forward_as_tuple(std::move(__f)), std::forward_as_tuple()) {}
+ _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f) : __func_(std::move(__f)), __alloc_() {}
- _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, const _Alloc& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(__f), std::forward_as_tuple(__a)) {}
+ _LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, const _Alloc& __a) : __func_(__f), __alloc_(__a) {}
_LIBCPP_HIDE_FROM_ABI explicit __alloc_func(const _Target& __f, _Alloc&& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(__f), std::forward_as_tuple(std::move(__a))) {}
+ : __func_(__f), __alloc_(std::move(__a)) {}
_LIBCPP_HIDE_FROM_ABI explicit __alloc_func(_Target&& __f, _Alloc&& __a)
- : __f_(piecewise_construct, std::forward_as_tuple(std::move(__f)), std::forward_as_tuple(std::move(__a))) {}
+ : __func_(std::move(__f)), __alloc_(std::move(__a)) {}
_LIBCPP_HIDE_FROM_ABI _Rp operator()(_ArgTypes&&... __arg) {
typedef __invoke_void_return_wrapper<_Rp> _Invoker;
- return _Invoker::__call(__f_.first(), std::forward<_ArgTypes>(__arg)...);
+ return _Invoker::__call(__func_, std::forward<_ArgTypes>(__arg)...);
}
_LIBCPP_HIDE_FROM_ABI __alloc_func* __clone() const {
typedef allocator_traits<_Alloc> __alloc_traits;
typedef __rebind_alloc<__alloc_traits, __alloc_func> _AA;
- _AA __a(__f_.second());
+ _AA __a(__alloc_);
typedef __allocator_destructor<_AA> _Dp;
unique_ptr<__alloc_func, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
- ::new ((void*)__hold.get()) __alloc_func(__f_.first(), _Alloc(__a));
+ ::new ((void*)__hold.get()) __alloc_func(__func_, _Alloc(__a));
return __hold.release();
}
- _LIBCPP_HIDE_FROM_ABI void destroy() _NOEXCEPT { __f_.~__compressed_pair<_Target, _Alloc>(); }
+ _LIBCPP_HIDE_FROM_ABI void destroy() _NOEXCEPT {
+ __func_.~_Fp();
+ __alloc_.~_Alloc();
+ }
_LIBCPP_HIDE_FROM_ABI static void __destroy_and_delete(__alloc_func* __f) {
typedef allocator_traits<_Alloc> __alloc_traits;
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 3cee48ef8538c6..87fd2984aabb9e 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -525,29 +525,29 @@ class __bucket_list_deallocator {
typedef allocator_traits<allocator_type> __alloc_traits;
typedef typename __alloc_traits::size_type size_type;
- __compressed_pair<size_type, allocator_type> __data_;
+ _LIBCPP_COMPRESSED_PAIR(size_type, __data_, allocator_type, __alloc_);
public:
typedef typename __alloc_traits::pointer pointer;
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator() _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)
- : __data_(0, __default_init_tag()) {}
+ : __data_(0) {}
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator(const allocator_type& __a, size_type __size)
_NOEXCEPT_(is_nothrow_copy_constructible<allocator_type>::value)
- : __data_(__size, __a) {}
+ : __data_(__size), __alloc_(__a) {}
_LIBCPP_HIDE_FROM_ABI __bucket_list_deallocator(__bucket_list_deallocator&& __x)
_NOEXCEPT_(is_nothrow_move_constructible<allocator_type>::value)
- : __data_(std::move(__x.__data_)) {
+ : __data_(std::move(__x.__data_)), __alloc_(std::move(__x.__alloc_)) {
__x.size() = 0;
}
- _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __data_.first(); }
- _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __data_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __data_; }
+ _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __data_; }
- _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __data_.second(); }
- _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __data_.second(); }
+ _LIBCPP_HIDE_FROM_ABI allocator_type& __alloc() _NOEXCEPT { return __alloc_; }
+ _LIBCPP_HIDE_FROM_ABI const allocator_type& __alloc() const _NOEXCEPT { return __alloc_; }
_LIBCPP_HIDE_FROM_ABI void operator()(pointer __p) _NOEXCEPT { __alloc_traits::deallocate(__alloc(), __p, size()); }
};
@@ -687,27 +687,27 @@ private:
// --- Member data begin ---
__bucket_list __bucket_list_;
- __compressed_pair<__first_node, __node_allocator> __p1_;
- __compressed_pair<size_type, hasher> __p2_;
- __compressed_pair<float, key_equal> __p3_;
+ _LIBCPP_COMPRESSED_PAIR(__first_node, __first_node_, __node_allocator, __node_alloc_);
+ _LIBCPP_COMPRESSED_PAIR(size_type, __size_, hasher, __hasher_);
+ _LIBCPP_COMPRESSED_PAIR(float, __max_load_factor_, key_equal, __key_eq_);
// --- Member data end ---
- _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __p2_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type& size() _NOEXCEPT { return __size_; }
public:
- _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __p2_.first(); }
+ _LIBCPP_HIDE_FROM_ABI size_type size() const _NOEXCEPT { return __size_; }
- _LIBCPP_HIDE_FROM_ABI hasher& hash_function() _NOEXCEPT { return __p2_.second(); }
- _LIBCPP_HIDE_FROM_ABI const hasher& hash_function() const _NOEXCEPT { return __p2_.second(); }
+ _LIBCPP_HIDE_FROM_ABI hasher& hash_function() _NOEXCEPT { return __hasher_; }
+ _LIBCPP_HIDE_FROM_ABI const hasher& hash_function() const _NOEXCEPT { return __hasher_; }
- _LIBCPP_HIDE_FROM_ABI float& max_load_factor() _NOEXCEPT { return __p3_.first(); }
- _LIBCPP_HIDE_FROM_ABI float max_load_factor() const _NOEXCEPT { return __p3_.first(); }
+ _LIBCPP_HIDE_FROM_ABI float& max_load_factor() _NOEXCEPT { return __max_load_factor_; }
+ _LIBCPP_HIDE_FROM_ABI float max_load_factor() const _NOEXCEPT { return __max_load_factor_; }
- _LIBCPP_HIDE_FROM_ABI key_equal& key_eq() _NOEXCEPT { return __p3_.second(); }
- _LIBCPP_HIDE_FROM_ABI const key_equal& key_eq() const _NOEXCEPT { return __p3_.second(); }
+ _LIBCPP_HIDE_FROM_ABI key_equal& key_eq() _NOEXCEPT { return __key_eq_; }
+ _LIBCPP_HIDE_FROM_ABI const key_equal& key_eq() const _NOEXCEPT { return __key_eq_; }
- _LIBCPP_HIDE_FROM_ABI __node_allocator& __node_alloc() _NOEXCEPT { return __p1_.second(); }
- _LIBCPP_HIDE_FROM_ABI const __node_allocator& __node_alloc() const _NOEXCEPT { return __p1_.second(); }
+ _LIBCPP_HIDE_FROM_ABI __node_allocator& __node_alloc() _NOEXCEPT { return __node_alloc_; }
+ _LIBCPP_HIDE_FROM_ABI const __node_allocator& __node_alloc() const _NOEXCEPT { return __node_alloc_; }
public:
typedef __hash_iterator<__node_pointer> iterator;
@@ -991,26 +991,34 @@ inline __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table() _NOEXCEPT_(
is_nothrow_default_constructible<__bucket_list>::value&& is_nothrow_default_constructible<__first_node>::value&&
is_nothrow_default_constructible<__node_allocator>::value&& is_nothrow_default_constructible<hasher>::value&&
is_nothrow_default_constructible<key_equal>::value)
- : __p2_(0, __default_init_tag()), __p3_(1.0f, __default_init_tag()) {}
+ : __size_(0), __max_load_factor_(1.0f) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const hasher& __hf, const key_equal& __eql)
- : __bucket_list_(nullptr, __bucket_list_deleter()), __p1_(), __p2_(0, __hf), __p3_(1.0f, __eql) {}
+ : __bucket_list_(nullptr, __bucket_list_deleter()),
+ __first_node_(),
+ __node_alloc_(),
+ __size_(0),
+ __hasher_(__hf),
+ __max_load_factor_(1.0f),
+ __key_eq_(__eql) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(
const hasher& __hf, const key_equal& __eql, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __hf),
- __p3_(1.0f, __eql) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(__hf),
+ __max_load_factor_(1.0f),
+ __key_eq_(__eql) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __default_init_tag()),
- __p3_(1.0f, __default_init_tag()) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __max_load_factor_(1.0f) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u)
@@ -1018,17 +1026,20 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u)
__bucket_list_deleter(allocator_traits<__pointer_allocator>::select_on_container_copy_construction(
__u.__bucket_list_.get_deleter().__alloc()),
0)),
- __p1_(__default_init_tag(),
- allocator_traits<__node_allocator>::select_on_container_copy_construction(__u.__node_alloc())),
- __p2_(0, __u.hash_function()),
- __p3_(__u.__p3_) {}
+ __node_alloc_(allocator_traits<__node_allocator>::select_on_container_copy_construction(__u.__node_alloc())),
+ __size_(0),
+ __hasher_(__u.hash_function()),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(__u.__key_eq_) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(const __hash_table& __u, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, __u.hash_function()),
- __p3_(__u.__p3_) {}
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(__u.hash_function()),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(__u.__key_eq_) {}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEXCEPT_(
@@ -1036,12 +1047,15 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEX
is_nothrow_move_constructible<__node_allocator>::value&& is_nothrow_move_constructible<hasher>::value&&
is_nothrow_move_constructible<key_equal>::value)
: __bucket_list_(std::move(__u.__bucket_list_)),
- __p1_(std::move(__u.__p1_)),
- __p2_(std::move(__u.__p2_)),
- __p3_(std::move(__u.__p3_)) {
+ __first_node_(std::move(__u.__first_node_)),
+ __node_alloc_(std::move(__u.__node_alloc_)),
+ __size_(std::move(__u.__size_)),
+ __hasher_(std::move(__u.__hasher_)),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(std::move(__u.__key_eq_)) {
if (size() > 0) {
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
- __u.__p1_.first().__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
+ __u.__first_node_.__next_ = nullptr;
__u.size() = 0;
}
}
@@ -1049,17 +1063,19 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u) _NOEX
template <class _Tp, class _Hash, class _Equal, class _Alloc>
__hash_table<_Tp, _Hash, _Equal, _Alloc>::__hash_table(__hash_table&& __u, const allocator_type& __a)
: __bucket_list_(nullptr, __bucket_list_deleter(__pointer_allocator(__a), 0)),
- __p1_(__default_init_tag(), __node_allocator(__a)),
- __p2_(0, std::move(__u.hash_function())),
- __p3_(std::move(__u.__p3_)) {
+ __node_alloc_(__node_allocator(__a)),
+ __size_(0),
+ __hasher_(std::move(__u.__hasher_)),
+ __max_load_factor_(__u.__max_load_factor_),
+ __key_eq_(std::move(__u.__key_eq_)) {
if (__a == allocator_type(__u.__node_alloc())) {
__bucket_list_.reset(__u.__bucket_list_.release());
__bucket_list_.get_deleter().size() = __u.__bucket_list_.get_deleter().size();
__u.__bucket_list_.get_deleter().size() = 0;
if (__u.size() > 0) {
- __p1_.first().__next_ = __u.__p1_.first().__next_;
- __u.__p1_.first().__next_ = nullptr;
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
+ __first_node_.__next_ = __u.__first_node_.__next_;
+ __u.__first_node_.__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
size() = __u.size();
__u.size() = 0;
}
@@ -1073,7 +1089,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::~__hash_table() {
static_assert((is_copy_constructible<hasher>::value), "Hasher must be copy-constructible.");
#endif
- __deallocate_node(__p1_.first().__next_);
+ __deallocate_node(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1119,8 +1135,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__detach() _NOEXCEPT {
for (size_type __i = 0; __i < __bc; ++__i)
__bucket_list_[__i] = nullptr;
size() = 0;
- __next_pointer __cache = __p1_.first().__next_;
- __p1_.first().__next_ = nullptr;
+ __next_pointer __cache = __first_node_.__next_;
+ __first_node_.__next_ = nullptr;
return __cache;
}
@@ -1137,10 +1153,10 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__move_assign(__hash_table& __u,
hash_function() = std::move(__u.hash_function());
max_load_factor() = __u.max_load_factor();
key_eq() = std::move(__u.key_eq());
- __p1_.first().__next_ = __u.__p1_.first().__next_;
+ __first_node_.__next_ = __u.__first_node_.__next_;
if (size() > 0) {
- __bucket_list_[std::__constrain_hash(__p1_.first().__next_->__hash(), bucket_count())] = __p1_.first().__ptr();
- __u.__p1_.first().__next_ = nullptr;
+ __bucket_list_[std::__constrain_hash(__first_node_.__next_->__hash(), bucket_count())] = __first_node_.__ptr();
+ __u.__first_node_.__next_ = nullptr;
__u.size() = 0;
}
}
@@ -1257,7 +1273,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__assign_multi(_InputIterator __f
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
__hash_table<_Tp, _Hash, _Equal, _Alloc>::begin() _NOEXCEPT {
- return iterator(__p1_.first().__next_);
+ return iterator(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1269,7 +1285,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::end() _NOEXCEPT {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
inline typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::const_iterator
__hash_table<_Tp, _Hash, _Equal, _Alloc>::begin() const _NOEXCEPT {
- return const_iterator(__p1_.first().__next_);
+ return const_iterator(__first_node_.__next_);
}
template <class _Tp, class _Hash, class _Equal, class _Alloc>
@@ -1281,8 +1297,8 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::end() const _NOEXCEPT {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
void __hash_table<_Tp, _Hash, _Equal, _Alloc>::clear() _NOEXCEPT {
if (size() > 0) {
- __deallocate_node(__p1_.first().__next_);
- __p1_.first().__next_ = nullptr;
+ __deallocate_node(__first_node_.__next_);
+ __first_node_.__next_ = nullptr;
size_type __bc = bucket_count();
for (size_type __i = 0; __i < __bc; ++__i)
__bucket_list_[__i] = nullptr;
@@ -1334,7 +1350,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique_perform(__node_po
// insert_after __bucket_list_[__chash], or __first_node if bucket is null
__next_pointer __pn = __bucket_list_[__chash];
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__nd->__next_ = __pn->__next_;
__pn->__next_ = __nd->__ptr();
// fix up __bucket_list_
@@ -1414,7 +1430,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi_perform(
size_type __bc = bucket_count();
size_t __chash = std::__constrain_hash(__cp->__hash_, __bc);
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__cp->__next_ = __pn->__next_;
__pn->__next_ = __cp->__ptr();
// fix up __bucket_list_
@@ -1499,7 +1515,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::__emplace_unique_key_args(_Key const&
// insert_after __bucket_list_[__chash], or __first_node if bucket is null
__next_pointer __pn = __bucket_list_[__chash];
if (__pn == nullptr) {
- __pn = __p1_.first().__ptr();
+ __pn = __first_node_.__ptr();
__h->__next_ = __pn->__next_;
__pn->__next_ = __h.get()->__ptr();
// fix up __bucket_list_
@@ -1677,7 +1693,7 @@ void __hash_table<_Tp, _Hash, _Equal, _Alloc>::__do_rehash(size_type __nbc) {
if (__nbc > 0) {
for (size_type __i = 0; __i < __nbc; ++__i)
__bucket_list_[__i] = nullptr;
- __next_pointer __pp = __p1_.first().__ptr();
+ __next_pointer __pp = __first_node_.__ptr();
__next_pointer __cp = __pp->__next_;
if (__cp != nullptr) {
size_type __chash = std::__constrain_hash(__cp->__hash(), __nbc);
@@ -1854,7 +1870,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::remove(const_iterator __p) _NOEXCEPT {
// Fix up __bucket_list_
// if __pn is not in same bucket (before begin is not in same bucket) &&
// if __cn->__next_ is not in same bucket (nullptr is not in same bucket)
- if (__pn == __p1_.first().__ptr() || std::__constrain_hash(__pn->__hash(), __bc) != __chash) {
+ if (__pn == __first_node_.__ptr() || std::__constrain_hash(__pn->__hash(), __bc) != __chash) {
if (__cn->__next_ == nullptr || std::__constrain_hash(__cn->__next_->__hash(), __bc) !=...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Thanks for meticulously tracking all this stuff down! It's a welcome change that's long overdue.
My only concern is the one that @huixie90 raised. I wonder if we can get away with not keeping the padding, since it's a very niche situation. Once it catches up to trunk, I'll try building Chromium OS without any padding and see what breaks (that could be a few weeks though, so we should consider other ways to address this).
| # define _LIBCPP_COMPRESSED_PAIR(T1, Name1, T2, Name2) \ | ||
| [[__gnu__::__aligned__(_LIBCPP_ALIGNOF(T2))]] _LIBCPP_NO_UNIQUE_ADDRESS T1 Name1; \ | ||
| _LIBCPP_COMPRESSED_PAIR_PADDING(T1, Name1##_padding_); \ | ||
| _LIBCPP_NO_UNIQUE_ADDRESS T2 Name2; \ |
There was a problem hiding this comment.
I have not looked in detail at the patch. Does this mean the _LIBCPP_NO_UNIQUE_ADDRESS affects the ABI?
There was a problem hiding this comment.
I think I'm missing something, but yes, [[no_unique_address]] affects the ABI.
There was a problem hiding this comment.
mainly that depending on the language version the ABI will be different.
There was a problem hiding this comment.
It won't be depending on the language version. [[no_unique_address]] always behaves the same.
There was a problem hiding this comment.
According to the __config _LIBCPP_NO_UNIQUE_ADDRESS can have different values. Since the attribute is C++20 this value can depend on the language version used, right? What am I missing?
There was a problem hiding this comment.
It can have in theory, but it doesn't. We can simplify that to
#if __has_cpp_attribute(msvc::no_unique_address)
# define _LIBCPP_NO_UNIQUE_ADDRESS [[msvc::no_unique_address]]
#else
# define _LIBCPP_NO_UNIQUE_ADDRESS [[no_unique_address]]
#endifThere was a problem hiding this comment.
A great, do you want to create a small PR for that?
There was a problem hiding this comment.
I'll change it after the release branch.
|
@huixie90 I don't think explicitly adding padding bytes is a problem here. The affected classes are in just about no way trivial, so I don't think it matters how the padding bytes are generated. @cjdb it would be great to get some data whether we could drop the padding bytes, but my suspicion is that we can't, since |
...cxx/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
Show resolved
Hide resolved
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
8d3053c
to
20c2a36
Compare
libcxx/include/__memory/shared_ptr.h
Outdated
| return __elem; | ||
| } | ||
| _LIBCPP_HIDE_FROM_ABI _Alloc* __get_alloc() _NOEXCEPT { return std::addressof(__alloc_); } | ||
| _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_CFI _Tp* __get_elem() _NOEXCEPT { return std::addressof(__elem_); } |
There was a problem hiding this comment.
I'm pretty sure you can (and should) drop _LIBCPP_NO_CFI here since this was only useful when we did type punning. It's fine to do in a separate PR but let's drop it.
libcxx/include/vector
Outdated
| @@ -725,8 +726,7 @@ public: | |||
| private: | |||
| pointer __begin_ = nullptr; | |||
| pointer __end_ = nullptr; | |||
| __compressed_pair<pointer, allocator_type> __end_cap_ = | |||
| __compressed_pair<pointer, allocator_type>(nullptr, __default_init_tag()); | |||
| _LIBCPP_COMPRESSED_PAIR(pointer, __cap_, allocator_type, __alloc_); | |||
There was a problem hiding this comment.
You should be able to restore the = nullptr here and re-simplify the ctors after my suggestion to the macro.
libcxx/test/libcxx/containers/sequences/deque/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/libcxx/containers/sequences/deque/abi.compile.pass.cpp
Outdated
Show resolved
Hide resolved
...cxx/utilities/memory/util.smartptr/util.smartptr.shared/libcxx.control_block_layout.pass.cpp
Outdated
Show resolved
Hide resolved
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
20c2a36
to
2f30123
Compare
|
I just discussed with @philnik777 and I realized that this requires support for |
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
from
2f30123
to
4807741
Compare
philnik777
force-pushed
the
compressed_pair_no_unique_address
branch
3 times, most recently
from
5d0b99d
to
da991f8
Compare
This patch is in preparation for the `__compressed_pair` refactor in llvm#76756. This gets the formatter tests to at least pass. Currently in draft because there's still some cleanup to be done.
|
I found an interesting interaction between this and the In short, That seems to be quite a rare case though. Upd: We worked around the issue locally. It's a problem with the lack of appropriate means to implement |
|
Now we found a number of other problems related to debuggability of the code under LLDB. Specifically, I wonder if debuggability of code using libc++ is considered an important trait of libc++, and this issue may be viewed as a reason for a revert (or to guard this with a feature macro)? For us it's definitely the case, but I see how priorities may be different for libc++ maintainers. Unfortunately, local revert of this commit proved to be problematic due to a number of later libc++ changes in the same files. |
LLDB was pinged about this a few months ago when this effort began. And we did the work to make sure the affected libc++ types are debuggable with this change. The LLDB test-suite runs cleanly (including all the libc++ specific tests). I'd say we did as much as we could to reduce fallout for debuggability. More insight into what exactly is breaking for you would be great. Is this |
Yes, we care about debuggability. However, I think debuggability should be better after this change since we don't use |
|
I am looking into this on our end. While I don't have an exact reproducer (yet), I can provide some details. Basically I think we're dealing with two problems:
|
Thanks for the details! Regarding (1), i think you might be running into: #97443. Which was causing issues for the |
|
No, I'm pretty sure that's not it. This is really just the vanilla (well, unstable abi vanilla) std::string from libc++.. which somehow ends up with alignment of one. |
I was just wondering where/how the alignment miscalculation is manifesting. Just based on this XFAILed test: #96932, it sounds like #97443 is indeed the issue. Could very well be a red herring though! |
|
Could be. I think that's the issue I am remembering. FWIW, the reproducer for this part is quite simple: |
|
#110648 is my attempt to fix the second problem. I don't yet know what's the deal with the first one. |
|
A standalone reproducer for the first part is: |
|
.. which is basically the same as the test case you linked to. And this is due to the "strange layout implies What was the outcome of that discussion? |
I'll have to go back over my discussion with @dwblaikie about this from couple of months ago. The resolution back then was to avoid having to rely on the miscalculated alignment when formatting types (basically side-stepping the issue). Not ideal, but worked for the libc++ formatters. To fix the alignment calculation, i think we would need to either (1) emit |
|
Based on the above, given that these are understood LLDB issues and that this libc++ change has several other benefits and has been almost a year in the making, I don't think reverting the libc++ change is the right thing to do in this case. |
That would be great. I've been ignoring that patch so far as the record layout builder is not my wheelhouse, but I'm going to comment on it now. |
…ion is not available (llvm#109693) This PR fixes the shared_ptr control block layout test that was recently updated in llvm#76756. When aligned allocation/deallocation is not available, part of the test doesn't work.
…ion is not available (llvm#109693) This PR fixes the shared_ptr control block layout test that was recently updated in llvm#76756. When aligned allocation/deallocation is not available, part of the test doesn't work.
…ion is not available (llvm#109693) This PR fixes the shared_ptr control block layout test that was recently updated in llvm#76756. When aligned allocation/deallocation is not available, part of the test doesn't work.
…lvm#108952) A few functions are now unnecessary, since we can access the members directly instread now.
…ion is not available (llvm#109693) This PR fixes the shared_ptr control block layout test that was recently updated in llvm#76756. When aligned allocation/deallocation is not available, part of the test doesn't work.
…lvm#108952) A few functions are now unnecessary, since we can access the members directly instread now.
This significantly simplifies the code, improves compile times and improves the object layout of types using
__compressed_pairin the unstable ABI. The only downside is that this is extremely ABI sensitive and pedantically breaks the ABI for empty final types, since the address of the subobject may change. The ABI of the whole object should not be affected.Fixes #91266
Fixes #93069